Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Use ArgAction::Set for enable-offchain-indexing flag #12521

Merged
merged 2 commits into from
Oct 18, 2022
Merged

Use ArgAction::Set for enable-offchain-indexing flag #12521

merged 2 commits into from
Oct 18, 2022

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Oct 18, 2022

Follow up for #12381
There are some places where we pass enable-offchain-indexing=true to binaries.

This does not work by default anymore in clap 4 which uses ArgAction::SetTrue as default. This means that the occurence of the flag sets the value to true and additional values are not allowed.

While I think the new approach makes more sense, there are some users of this flag and we should not break it if not necessary.

@skunert skunert added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Oct 18, 2022
@skunert skunert requested a review from a team October 18, 2022 13:55
@skunert skunert changed the title Use ArgAction::Set for enable-offchain-indexing Use ArgAction::Set for enable-offchain-indexing flag Oct 18, 2022
@skunert skunert added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Oct 18, 2022
koute
koute approved these changes Oct 18, 2022
View reviewed changes
Copy link
Contributor

@koute koute left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other places where people might have reasonably used the --arg=true form? Maybe we should do this everywhere? (I'm not necessarily suggesting we should; just thinking out loud.)

michalkucharczyk
michalkucharczyk approved these changes Oct 18, 2022
View reviewed changes
Copy link
Contributor

@michalkucharczyk michalkucharczyk left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to indicate that --flag=true is deprecated (when used)?

@skunert
Copy link
Contributor Author

skunert commented Oct 18, 2022

Are there any other places where people might have reasonably used the --arg=true form? Maybe we should do this everywhere? (I'm not necessarily suggesting we should; just thinking out loud.)

I think this is a non-issue for our other flags. I investigated a bit and this specific flag has specified a value_name in the clap attribute. This attribute is what required a value to be present previously (you can not do the same on our other bool flags. e.g. --validator=true is not allowed) . I was also thinking of just changing this, but a search on github found several deployment files and our devops repo also has a few occurences.

Is there a way to indicate that --flag=true is deprecated (when used)?

We could take a look at the raw arguments and look for this specifically, but there is no clap-specific way to look for this.

@skunert
Copy link
Contributor Author

skunert commented Oct 18, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 487ac5c into paritytech:master Oct 18, 2022
@skunert skunert mentioned this pull request Oct 18, 2022
Merged
@AurevoirXavier AurevoirXavier mentioned this pull request Jan 10, 2023
Closed
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@skunert @ark0f
Use ArgAction::Set for enable-offchain-indexing flag (paritytech#12521
e1cc73f 
)

* Use ArgAction::Set for enable-offchain-indexing

* Provide default value for `enable-offchain-indexing`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@michalkucharczyk michalkucharczyk michalkucharczyk approved these changes

@koute koute koute approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
SDK Node
Status: done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skunert @koute @michalkucharczyk